Repository navigation
Support integer indexing of Categorical and MultivariateNormal predictions - #442
Open
alejandroschuler wants to merge 6 commits into
Open
alejandroschuler wants to merge 6 commits into
alejandroschuler wants to merge 6 commits into
Conversation
Distn.__getitem__ and Distn.__len__ read self._params. NormalFixedVar and NormalFixedMean override __init__ without calling the base constructor, so _params stayed None. On the output of pred_dist, dist[0:2] raised "TypeError: 'NoneType' object is not subscriptable" and len(dist) raised an AttributeError. Both constructors now set self._params, as LogNormal and Exponential do. A parametrized test builds every distribution in ngboost.distns.__all__ and checks len, slicing and integer indexing. Only the two fixed Normal classes failed on master. Integer indexing is marked xfail for Bernoulli, k_categorical and MultivariateNormal, which fail for a different reason (their constructors need two-dimensional params). Found while assessing #168. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…tions Distn.__getitem__ passes params[:, i] to the constructor for an integer key, so the constructor gets a one-dimensional array. The constructors of the k_categorical classes (Bernoulli included) and of MultivariateNormal read params.shape[1], so dist[0] raised an error. Slices worked. The constructors now accept params without the row axis, and the methods that used fixed axis numbers use the last axes instead. dist[i] is then the distribution of row i without the row axis, as for the other distributions and as for a numpy array: class_probs() has shape (K,), predict() gives one class, MultivariateNormal has loc (p,) and cov (p, p), logpdf(y) gives a scalar, and scipy_distribution() gives one scipy object. dist[i:i+1] still gives a batch of one row. ClassificationDistn.predict takes the argmax over the last axis, which is the same as axis 1 for a batch. For batches the outputs are unchanged. The integer-index test no longer marks these three classes as xfail. New tests check class_probs, predict, sample, logpdf, cov, scipy_distribution and iteration on dist[i], and dist[0] of fitted NGBClassifier and NGBRegressor models against predict_proba and predict. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
#438 documented params for a batch of N rows. dist[i] gives loc of shape p and cov of shape p x p. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Depends on #441. This branch starts from
fix/normal-fixed-params, because #441 adds the indexing tests that this PR changes. Until #441 merges, the diff here also shows its commit. Review the last commit only, or wait for #441.dist[0](an integer key) raised an error forBernoulli,k_categorical(k)andMultivariateNormal(k).Distn.__getitem__passesparams[:, i]to the constructor, which is a one-dimensional array for an integer key. The constructors of these classes readparams.shape[1]. Slices such asdist[1:4]worked. The other distributions inngboost.distnsaccept an integer key.Design
The fix is in the constructors.
dist[i]is now the distribution of row i without the row axis, as for the other distributions and as for a numpy array:Categorical.__init__builds the logits with shape(K,) + params.shape[1:], andsample1drawsprobs.shape[1:]random numbers in place of(1, len(self)).MultivariateNormalandget_chol_factoruse...and the last axes in place of fixed axis numbers in__init__,summaries,logpdfandrv.scipy_distribution()gives one scipy object for one row and a list for a batch.ClassificationDistn.predicttakes the argmax over the last axis. For a batch this is the same as axis 1.I also looked at two other places for the fix. An override of
__getitem__in the three classes, or a change inDistn.__getitem__, would passparams[:, [i]]and keep the row axis. Thendist[i]would be the same asdist[i:i+1], a batch of one row, withclass_probs()of shape(1, K)and aMultivariateNormallocof shape(1, p). ForNormal,dist[0].locis a scalar, so these three classes would differ from the rest. The test from #441 comparesdist[i].paramswith row i ofdist.params, andMultivariateNormalwould fail it. A change inDistn.__getitem__would also need a check of the class, so thatdist[i]stays the same for the classes that work now. The constructor fix needs no such check, and it does not touchDistn.__getitem__or theDistn.__getattr__that #433 adds.For a batch, the outputs do not change. I compared the old and new code on batches of 7 rows for
k_categorical(2),k_categorical(3),MultivariateNormal(2)andMultivariateNormal(3):class_probs,predict, seededsample,loc,cov,logpdf,summaries, and theLogScored_scoreandmetricof the manifold are bit-identical. A batch of one row (dist[0:1]) also gives the samelogpdfand seededsampleas before.Example
d = Dist(np.random.default_rng(0).normal(scale=0.3, size=(Dist.n_params, 4))), thend[0]:DistBernoulliValueError: not enough values to unpack (expected 2, got 1)class_probs()shape(2,),predict()one classk_categorical(3)ValueErrorclass_probs()shape(3,),predict()one classMultivariateNormal(2)IndexError: tuple index out of rangeloc(2,),cov(2, 2),logpdf(y)a scalarIteration (
for row in dist) now works for these classes too, because it calls__getitem__with 0, 1, 2 and so on.One difference stays.
BetaBernoullicomes from the sympy factory, and itsdist[0].class_probs()has shape(1, 2)because itsclass_probscallsnp.column_stack. This PR does not change it.Tests
test_integer_index_keeps_class_and_paramsno longer marksBernoulli,k_categoricalandMultivariateNormalas xfail, and the unused_NO_INT_INDEXset is gone.test_categorical_integer_index_gives_one_rowchecksclass_probs,predictandsampleofdist[2]anddist[-1]against the batch, and iteration over the rows.test_multivariatenormal_integer_index_gives_one_rowcheckspredict,cov,logpdf,scipy_distributionandsamplefor k = 2 and 3, andlogpdfof one row at several points.test_integer_index_of_fitted_classifier_and_mvncheckspred_dist(X)[0]of a fittedNGBClassifieragainstpredict_probaandpredict, and of a fittedNGBRegressorwithMultivariateNormal(2)againstpredictandlogpdf.--slow: 244 passed, 1 skipped, 2 xfailed, 22 xpassed (236 passed, 1 skipped, 5 xfailed, 22 xpassed on Store params in NormalFixedVar and NormalFixedMean so they can be sliced #441). The other 2 xfailed and 22 xpassed cases come from the xfail mark ontest_dists_runs_on_examples_logscore, which this PR does not touch (Mark only the T case as xfail in the logscore distribution test #437 changes it).black,isort,flake8andpylintare clean on the changed files.🤖 Generated with Claude Code